Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APBoardConfig: Add skip validate board #23620

Merged

Conversation

haydendonald
Copy link
Contributor

Added the ability to skip board validation to the BRD_OPTIONS param. This will allow the board to continue on board errors.

@khancyr
Copy link
Contributor

khancyr commented Apr 27, 2023

I don't think this is necessary as we can control this from hwdef and only Cubes are using this.
What is the main use case of this ?

@IamPete1
Copy link
Member

Do you have a example that fails when it should pass? Better to fix that case than to bypass the check.

@haydendonald
Copy link
Contributor Author

Hi @IamPete1 @khancyr. This is mainly for our testing, we are looking to use LUA to do testing.

This came up as one of our Cubes that has a failed IMU would boot, thus not starting the LUA testing to report what has actually failed.

The other thought was that this could be a useful feature for people that don't care about IMUs for example on a faulty board, not ideal but an option.

Added the ability to ignore board validation. This has been added to the 7th bit in BRD_OPTIONS
@peterbarker
Copy link
Contributor

@haydendonald I'm not sure I agree with your reasoning.

By the time you're dropping these sensors you're not really running on e.g. a CubeOrange any more, you're running on custom hardware. As @khancyr points out, you can create a new hardware definition which doesn't have the relevant IMU inside.

Removing the C++ check that your board looks good just so that you can add a LUA check also seems somewhat suspect reasoning :-) If a check gets added behind the same define and you have disabled it on all of your boards so your LUA script catches things then you won't get the benefit of the new check.

This sort of thing isn't free - everyone now gets to wade through another parameter option, and everybody pays a flash cost. As this PR currently stands, this option bit is costing 744 bytes on CubeOrange! I really think that last bit needs explaining before merge.

@bugobliterator
Copy link
Member

bugobliterator commented May 15, 2023

@peterbarker the requirement for this option bit is mostly with regards to factory testing of CubePilot hardware. We don't want to run custom firmware in factory, but keep it up to date with latest releases.

Currently without ignore the lua script will fail to run and report issues in formatted way that we want. That is the main reason for this option bit.

@IamPete1
Copy link
Member

I tend to agree with @peterbarker, this is quite a big hammer to let everyone loose with. You could do this from a custom testing hwdef I think? Just include CubeOrange and undef BOARD_VALIDATE? Would still be a bit custom of course, but not as much as carrying this patch. I think you would need a custom hwdef in anycase to set this parm? I would be surprised if scripting loads before the config error loop and is able to set the param.

@tridge tridge merged commit 10038a6 into ArduPilot:master May 15, 2023
82 checks passed
@tridge
Copy link
Contributor

tridge commented May 15, 2023

I've wanted this for a while, it is very annoying when you can't tell what sensors are present

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants